Adopt Kubernetes style TLS Secret #597

somtochiama · 2023-08-09T12:45:05Z

Deprecate the usage of caFile for the .spec.certSecretRef secret in Provider. Both caFile and ca.crt keys are supported for Provider with the latter taking precedence.

Part of: fluxcd/flux2#4137

darkowlzz · 2023-08-16T15:46:28Z

internal/controller/testdata/certs/ca-key.pem

Would be good to include a makefile that generates this and the other files in this directory.
Similar to https://github.com/fluxcd/source-controller/blob/aa370f284db70e09d8acb49d5bed4e42908b8cb3/internal/controller/testdata/certs/Makefile#L17-L20 .

makkes · 2023-08-16T15:51:35Z

internal/controller/alert_controller.go

@@ -178,7 +178,7 @@ func (r *AlertReconciler) isProviderReady(ctx context.Context, alert *apiv1beta2
 	providerName := types.NamespacedName{Namespace: alert.Namespace, Name: alert.Spec.ProviderRef.Name}
 	if err := r.Get(ctx, providerName, provider); err != nil {
 		// log not found errors since they get filtered out
-		ctrl.LoggerFrom(ctx).Error(err, "failed to get provider %s", providerName.String())
+		ctrl.LoggerFrom(ctx).Error(err, fmt.Sprintf("failed to get provider '%s'", providerName.String()))


In favor of structured logging I suggest:

Suggested change

ctrl.LoggerFrom(ctx).Error(err, fmt.Sprintf("failed to get provider '%s'", providerName.String()))

ctrl.LoggerFrom(ctx).Error(err, "failed to get provider", "provider", providerName.String())

makkes · 2023-08-16T15:52:50Z

internal/controller/provider_controller.go

-			return fmt.Errorf("no caFile found in secret %s", provider.Spec.CertSecretRef.Name)
+			caFile, ok = secret.Data["caFile"]
+			if !ok {
+				return fmt.Errorf("no `ca.crt` key found in secret '%s'", provider.Spec.CertSecretRef.Name)


Suggested change

return fmt.Errorf("no `ca.crt` key found in secret '%s'", provider.Spec.CertSecretRef.Name)

return fmt.Errorf("no 'ca.crt' nor 'caFile' key found in secret", "secret", provider.Spec.CertSecretRef.Name)

I am not sure if we want to add the deprecated key caFile to the log so as not to encourage users to use it.

Yeah, had the same thought. Up to you.

makkes · 2023-08-16T15:53:23Z

internal/controller/provider_controller.go

+			if !ok {
+				return fmt.Errorf("no `ca.crt` key found in secret '%s'", provider.Spec.CertSecretRef.Name)
+			}
+			log.Info("warning: specifying CA cert via `caFile` is deprecated, please use `ca.crt` instead")


Suggested change

log.Info("warning: specifying CA cert via `caFile` is deprecated, please use `ca.crt` instead")

log.Info("warning: specifying CA cert via 'caFile' is deprecated, please use 'ca.crt' instead")

makkes · 2023-08-16T15:55:08Z

internal/server/event_handlers.go

-					continue
+					caFile, ok = secret.Data["caFile"]
+					if !ok {
+						alertLogger.Error(nil, "no `ca.crt` key found in secret '%s'", provider.Spec.CertSecretRef.Name)


Suggested change

alertLogger.Error(nil, "no `ca.crt` key found in secret '%s'", provider.Spec.CertSecretRef.Name)

alertLogger.Error(nil, "no 'ca.crt' nor 'caFile' key found in secret", "secret", provider.Spec.CertSecretRef.Name)

aryan9600 · 2023-08-18T05:52:30Z

docs/spec/v1beta2/providers.md

@@ -1042,7 +1042,7 @@ Secret in the same namespace as the Provider, containing the TLS CA certificate.
 #### Example

 To enable notification-controller to communicate with a provider API over HTTPS
-using a self-signed TLS certificate, set the `caFile` like so:
+using a self-signed TLS certificate, set the `ca.crt` like so:


we also need to add a deprecation notice for the caFile key like the one here: https://github.com/fluxcd/source-controller/blob/main/docs/spec/v1beta2/helmrepositories.md#secret-reference

my bad, skipped it somehow but the same goes for the API docs

darkowlzz · 2023-08-23T11:29:15Z

docs/spec/v1beta2/providers.md

+**Note:** Support for the `caFile` key have been
+deprecated. If you have any Secrets using these keys,


Suggested change

**Note:** Support for the `caFile` key have been

deprecated. If you have any Secrets using these keys,

**Warning:** Support for the `caFile` key has been

deprecated. If you have any Secrets using this key,

Also, maybe mention the types of Secret that's allowed in the above section, not part of this warning.

aryan9600

lgtm!

Signed-off-by: Somtochi Onyekwere <somtochionyekwere@gmail.com>

stefanprodan

LGTM

Thanks @somtochiama 🏅

somtochiama requested a review from aryan9600 August 9, 2023 12:45

aryan9600 mentioned this pull request Aug 9, 2023

Support Kubernetes style TLS Secrets fluxcd/flux2#4137

Closed

4 tasks

somtochiama marked this pull request as ready for review August 14, 2023 13:51

darkowlzz mentioned this pull request Jun 15, 2023

Refactor events and introduce v1beta3 API for Alert and Provider #540

Merged

darkowlzz reviewed Aug 16, 2023

View reviewed changes

makkes reviewed Aug 16, 2023

View reviewed changes

aryan9600 requested changes Aug 18, 2023

View reviewed changes

somtochiama force-pushed the kube-tls-secret branch from 015ebc4 to 0ac4494 Compare August 23, 2023 10:36

somtochiama requested a review from makkes August 23, 2023 10:38

darkowlzz reviewed Aug 23, 2023

View reviewed changes

aryan9600 approved these changes Aug 23, 2023

View reviewed changes

use kubernetes tls style secret in provider

23e733b

Signed-off-by: Somtochi Onyekwere <somtochionyekwere@gmail.com>

somtochiama force-pushed the kube-tls-secret branch from 794d100 to 23e733b Compare August 23, 2023 13:06

stefanprodan added the area/alerting Alerting related issues and PRs label Aug 23, 2023

stefanprodan approved these changes Aug 23, 2023

View reviewed changes

stefanprodan merged commit c04ad84 into main Aug 23, 2023
7 checks passed

stefanprodan deleted the kube-tls-secret branch August 23, 2023 13:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adopt Kubernetes style TLS Secret #597

Adopt Kubernetes style TLS Secret #597

somtochiama commented Aug 9, 2023

darkowlzz Aug 16, 2023

makkes Aug 16, 2023

makkes Aug 16, 2023

somtochiama Aug 16, 2023 •

edited

Loading

makkes Aug 16, 2023

makkes Aug 16, 2023

makkes Aug 16, 2023

aryan9600 Aug 18, 2023

aryan9600 Aug 18, 2023

darkowlzz Aug 23, 2023

darkowlzz Aug 23, 2023

aryan9600 left a comment

stefanprodan left a comment

	ctrl.LoggerFrom(ctx).Error(err, fmt.Sprintf("failed to get provider '%s'", providerName.String()))
	ctrl.LoggerFrom(ctx).Error(err, "failed to get provider", "provider", providerName.String())

	return fmt.Errorf("no `ca.crt` key found in secret '%s'", provider.Spec.CertSecretRef.Name)
	return fmt.Errorf("no 'ca.crt' nor 'caFile' key found in secret", "secret", provider.Spec.CertSecretRef.Name)

	log.Info("warning: specifying CA cert via `caFile` is deprecated, please use `ca.crt` instead")
	log.Info("warning: specifying CA cert via 'caFile' is deprecated, please use 'ca.crt' instead")

	alertLogger.Error(nil, "no `ca.crt` key found in secret '%s'", provider.Spec.CertSecretRef.Name)
	alertLogger.Error(nil, "no 'ca.crt' nor 'caFile' key found in secret", "secret", provider.Spec.CertSecretRef.Name)

		Note: Support for the `caFile` key have been
		deprecated. If you have any Secrets using these keys,

Adopt Kubernetes style TLS Secret #597

Adopt Kubernetes style TLS Secret #597

Conversation

somtochiama commented Aug 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

somtochiama Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aryan9600 left a comment

Choose a reason for hiding this comment

stefanprodan left a comment

Choose a reason for hiding this comment

somtochiama Aug 16, 2023 •

edited

Loading